-
Notifications
You must be signed in to change notification settings - Fork 1
feat: IConfiguration interface for config access in EppoClient #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏👏👏 BRAVO 👏👏👏
This is MUCH cleaner 🧹 🧹 🧹 Huge improvement for these files and I really appreciate the additional tests as well 💪 💪 💪
👨🍳 👌
}); | ||
|
||
describe('getConfiguration', () => { | ||
it('should return an empty configuration instance before a config has been loaded', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
jest.restoreAllMocks(); | ||
}); | ||
|
||
describe('getConfiguration', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
private readonly banditModelConfigurationStore: IConfigurationStore<BanditParameters> | null, | ||
) {} | ||
) { | ||
this.configuration = new StoreBackedConfiguration( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
import { BanditParameters, BanditVariation, Environment, Flag, ObfuscatedFlag } from './interfaces'; | ||
import { BanditKey, FlagKey } from './types'; | ||
|
||
describe('StoreBackedConfiguration', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
IHttpClient, | ||
IUniversalFlagConfigResponse, | ||
} from './http-client'; | ||
import { StoreBackedConfiguration } from './i-configuration'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this i-* file pattern for interfaces. Helps separate out functionally related interfaces, vs putting them all in one large file
Thank you, thank you very much ?: |
labels: mergeable
Fixes: #issue
Motivation and Context
Accessing the underlying configuration values is complex (it relies on several
IConfigurationStore
instances) and as a result, requires a couple of one-off orphan functions and oddly located helper methods to interact with the configuration objects. All of this implementation detail leaks into the EppoClientDescription
new interface:
An instance of this interface is available from the
ConfigurationRequestor
(a default instance is created if the config requestor has not been instantiated yet).Each call into the EppoClient that requires config access first gets the current IConfiguration then uses that config object throughout the callstack.
For now, the
IConfiguration
instance is backed by the various config stores, so all this PR does is move access from the stores directly to corresponding methods on theIConfiguration
instance.How has this been tested?
What's left to do
Because the config is backed by the stores which are written to by the
ConfigRequestor
, the configuration can actually change during a call-in computation. As such, the next step in this process will be to introduce aReadOnlyConfiguration
that will be a snapshot of the config instead of a life object.